Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ingest Manger] Return promise from plugin#start #69089

Closed
wants to merge 100 commits into from

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Jun 12, 2020

⚠️ Sorry for the notification spam ⚠️

Moved to #69505

Problem

The Ingest Manager public start method is causing timeout errors for people not even directly using the plugin #66125

Root cause

Doing an await in the start lifecycle method blocks all of Kibana from progressing until it's complete

Proposed solution

  1. Do not await in the public start lifecycle method. Fixes [Ingest][EPM] Ingest Manager start lifecycle didn't finish in 30 secs #66125
    PR based on [Ingest][EPM] Ingest Manager start lifecycle didn't finish in 30 secs #66125 (comment) & [Ingest][EPM] Ingest Manager start lifecycle didn't finish in 30 secs #66125 (comment)
  2. Change success to be Promise
-  success: boolean;
-  error?: {
-    message: string;
-  };
+  success: Promise<boolean>;

Why this way

re (1): while great gains were made in setup time, error reports are still coming. Dropping the await prevents the possibility of any timeout errors. It's also the Platform team's preference.

re (2): the Ingest Manager plugin currently returns

  1. one property to express if the plugin successfully initialized success: boolean
  2. one property which gives details if an error occurred error?

We can avoid defining our own by using things already available on the Promise interface

  1. .then for the success case
  2. .catch for the error case which includes and an Error instance

I think we should limit the scope of the changes we make in the PRs backported to 7.8. This PR gives the most impact (makes it impossible to block/fail Kibana if the setup takes too long) with the least change.

The change to the return value is technically a breaking change for our consumers but any client side code that depended on setup completing, can now await and still get that guarantee. I believe this is the change required:

-    if (!ingestManager.success) {
-      if (ingestManager.error) {
-        displayToastWithModal(ingestManager.error.message);
-      } else {
-        displayToast();
-      }
-    }
+    ingestManager.success
+      .catch((error: Error) => displayToastWithModal(error.message));

This is only client-side, what about the server

Any server side timing issues are present in master already and should be discussed and addressed in follow up PRs. I did some exploration of that in #68631

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@jfsiii
Copy link
Contributor Author

jfsiii commented Jun 14, 2020

@elasticmachine merge upstream

@jfsiii jfsiii changed the title WIP. Pushing for CI [Ingest Manger] Return promise from plugin#start Jun 14, 2020
@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 labels Jun 15, 2020
@jfsiii jfsiii marked this pull request as ready for review June 15, 2020 00:29
@jfsiii jfsiii requested a review from a team as a code owner June 15, 2020 00:29
@jfsiii jfsiii requested a review from a team June 15, 2020 00:29
@jfsiii jfsiii requested a review from a team as a code owner June 15, 2020 00:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

kertal and others added 5 commits June 15, 2020 08:44
…in registry function (elastic#68642)

* creating observability context registry

* adding registryContext

* addressing PR comments

* addressing PR comments

* adding context to registry provider

* adding obs own registry

* refactoring

* refactoring

* fixing types

* removing apm code

Co-authored-by: Elastic Machine <[email protected]>
…lastic#69116)

* Call sync search recursively

* Fix test

* Fix search mock to avoid resetting it

* delete empty line

* fix tests
@ruflin
Copy link
Member

ruflin commented Jun 15, 2020

@jfsiii Two questions

  • If a user the first time visits Kibana and goes very quickly to Ingest Manager, will the Ingest Manager UI block?
  • What happens in the API case if someone calls /setup? Will he get an immediate reponse?

In general +1 on moving forward with this.

alisonelizabeth and others added 3 commits June 15, 2020 08:48
…lastic#68991)

* scrub the lead and trailing brackets from ipv6 host names

* Update comment

* refactor: scrub -> sanitize

Co-authored-by: Elastic Machine <[email protected]>
@jfsiii jfsiii added the v8.0.0 label Jun 15, 2020
@jfsiii jfsiii self-assigned this Jun 15, 2020
* rename server plugin to siem to avoid privilege issue

* review from alerting

* missing change with rename

* fix tests

* missing api integration test

* fix api integration spaces
mshustov and others added 3 commits June 15, 2020 15:32
* make browserAsync type safe

* adopt tests

* prefer unknown over any

* simplify signature
…67783)

Added example for using dashboard container by value
1.1 Refactored embeddable explorer e2e test to use new example, removed not needed kbn_tp_embeddable_explorer plugin.
For embeddable explorer examples went away from using getFactoryById() to improve type checks
There is new component a replacement for EmbeddableFactoryRenderer with slightly more flexible api: EmbeddableRenderer.
3.1 We can improve it going forward to support more use case
@jfsiii jfsiii requested review from a team as code owners June 18, 2020 13:05
@jfsiii jfsiii requested a review from a team June 18, 2020 13:05
@jfsiii jfsiii requested review from a team as code owners June 18, 2020 13:05
@jfsiii jfsiii requested a review from a team June 18, 2020 13:05
@jfsiii jfsiii requested a review from a team as a code owner June 18, 2020 13:05
@botelastic botelastic bot added Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Jun 18, 2020
@jfsiii jfsiii marked this pull request as draft June 18, 2020 13:07
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ingestManager 360 +1 359

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jfsiii jfsiii removed Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Jun 18, 2020
@jfsiii
Copy link
Contributor Author

jfsiii commented Jun 18, 2020

Closing in favor of #69505

@jfsiii jfsiii closed this Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ingest][EPM] Ingest Manager start lifecycle didn't finish in 30 secs